Skip to content

Hover on fills #673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jun 22, 2016
Merged

Hover on fills #673

merged 17 commits into from
Jun 22, 2016

Conversation

alexcjohnson
Copy link
Collaborator

Provides a new attribute for scatter and scatterternary traces: hoveron, which can be:

  • 'points': the normal behavior: hovering selects a single point in each trace, and shows you the point data.
  • 'fills': new behavior: hovering over a region selects the whole region - not necessarily the whole trace, if it's disjoint. This works similarly to heatmap hover in that (a) it ignores "compare" hovermodes and only ever shows the region you are actually over, and (b) it never outweighs a point you could be hovering over (from another trace). The label only shows the trace text or name. Is there anything else we would want there?.

hoveron defaults to 'points' unless there are no markers or text points AND fill mode is one of the "region fills" 'toself' or 'tonext'. The other fills (tozero|tonext)(x|y) have a different real meaning, as they effectively fill from each individual point to another point or to the axis, so these keep hoveron='points' by default.

In scatter:
screen shot 2016-06-20 at 5 10 32 pm

and scatterternary:
screen shot 2016-06-20 at 8 34 35 pm

Notice that both the hoverable region and the label position are calculated by making polygons out of the trace points, so they won't be able to match smoothed edges. Most of the time that's a small error, but you could imagine cases where it becomes a big deal. Fixing the hoverable region would be possible presumably by allowing the fills to capture events, though that would make the system less flexible when combined with other traces. Fixing it for label position (right now the label is vertically at the midpoint of the hovered-on region, and horizontally at the rightmost (or leftmost) edge of the polygon) is something I'm not even technically sure how to do... some sort of search for the last hoverable pixel based on synthetic events? Anyway, these are cumbersome fixes for what's normally a minor or non issue, so I propose to ignore it.

if you make this plot, you could never hover on the scatter points:
Plotly.newPlot(gd, [
    {x:[1,2,3], y: [1,4,7], z:[[1,2,3],[4,5,6],[7,8,9]], type:'heatmap'},
    {x:[1,2,3], y:[1,4,7]}
],{hovermode:'closest'})
Fx.MAXDIST had moved so sorting point distance was broken with heatmaps.
Finds the outer crossing points at the vertical midline.
Note that this will miss curves, if the lines are smoothed, and
unlike the contains test there isn't a clean way (far as I know)
to get it using browser machinery.
var Lib = require('../../lib');


module.exports = function hoverPoints(pointData, xval, yval, hovermode, contour) {
// never let a heatmap override another type as closest point
if(pointData.distance < Fx.MAXDIST) return;
if(pointData.distance < constants.MAXDIST) return;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etpinard I didn't notice you fixed this issue in #655 too - but when I merged master back in I used your version, which is cleaner.

dx = Math.abs(xa.c2p(di.x) - xa.c2p(xval)),
dy = Math.abs(ya.c2p(di.y) - ya.c2p(yval));
dx = xa.c2p(di.x) - xpx,
dy = ya.c2p(di.y) - ypx;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dx, dy, and dxy are the hot path for scatter hover, this should make them a little faster.

@@ -47,7 +47,7 @@ describe('Test click interactions:', function() {
setTimeout(function() {
click(x, y);
setTimeout(function() { resolve(); }, DBLCLICKDELAY / 2);
}, DBLCLICKDELAY / 4);
}, DBLCLICKDELAY / 2);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seem to be intermittent errors with click and doubleclick returning before the plot gets a chance to redraw. It would be better to ask the plot itself when it's done but I'm not sure how. We could try to tap into gd._promises but I'm not sure we'd know when the plot has started redrawing so that might be meaningless...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you fixed #567 Thanks!

Maybe we could listen to plotly_afterplot ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly. Though there may be cases where there never is a Plotly.plot call after the click.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#567 is not fixed... still some intermittent failures around there. I'm not going to tweak it any more in this PR, but I think you're on to something with judicious event listening.

@alexcjohnson
Copy link
Collaborator Author

@etpinard I converted hoveron to a flaglist, and wrapped the test mocks in deepExtend. Note that the hover label test verifies that 'points+fills' indeed generates exactly one label of either type.

Anything else?

@etpinard
Copy link
Contributor

No. That should do it! I'll test things out one more time tonight or tomorrow am

@@ -63,6 +66,11 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
if(!subTypes.hasLines(traceOut)) handleLineShapeDefaults(traceIn, traceOut, coerce);
}

if(traceOut.fill === 'tonext' || traceOut.fill === 'toself') {
dfltHoverOn.push('fills');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having second thoughts about making hoveron: 'points+fills' the default for scatter (my mistake).

With hovermode: 'x' by default the 'fills' in hoveron: 'points+fills' is hardly noticeable for most polygons.

Moreover, overriding the layout.hovermode default based on fill would a little too intrusive I think.

While hovermode: 'closest' by default for scatterternary, I'm not sure it warrants a different default than its scatter cousin.

So, I'm leaning towards making hoveron: 'points' the default for scatter and scatterternary regardless of the fill value.

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ternary is easier here because it doesn't even have a hovermode: 'x'... But I'd also argue that if you have a scatter trace with fill tonext or toself, you're making the statement "I'm enclosing an area" which doesn't make much sense in conjunction with comparing different traces at the same x value. For one, your outline trace is itself generally going to have two (or more) points at the same x value (or 2 crossings of the same x value anyway, on opposite sides of the region) which is a situation that has always confused people ("why do I never see hover data for the second point?")

Not sure if there's an issue about it, but we've talked in the past about using trace info to make a smarter default hovermode choice, and I think this makes that case even stronger. hovermode: 'x' should only really be the default when you have multiple traces all with monotonically increasing (or decreasing) x (or position, like horizontal bars) data. I don't think that would be too intrusive, hovermode is a layout property but it is all about exposing the trace data anyway. If we did that, this concern would take care of itself.

I suppose in the meantime we could put hoveron back to the way it was before as a default: only have hoveron: 'fills' if you have an area fill and no explicit points, otherwise hoveron: 'points', never default to both. But I still think I'd prefer to leave it as is now, because it matches the meaning implied by your trace attributes.

@jackparmer
Copy link
Contributor

cc @chriddyp @VeraZab for Plotly2 integration

@etpinard
Copy link
Contributor

💃 💃 💃

@alexcjohnson alexcjohnson merged commit 567ba90 into master Jun 22, 2016
@alexcjohnson alexcjohnson deleted the hoveron-fill branch June 22, 2016 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants